Add parameter for service account name to all ackgenerate methods #270

acornett21 · 2022-01-19T22:21:30Z

Issue #, if available:

Fixes: Add support for service-specific service accounts in place of default in generated kustomize manifests community#777

Description of changes:
The goal of this is to move away from using the default service account. It seems this might have been done with the helm charts/templates, but was not ported to the Kustomization files. This means each operator/controller would have its own service account. Things that changed are below:

Refactored build so that ServiceAccountName is available in ack-generate controller process as well as ack-generate release process
added in /rbac/service-account.yaml.tpl to create a service account, and then referece this account in the Deployment and ClusterRoleBinding
Refactored build/release sh files to add in ACK_GENERATE_SERVICE_ACCOUNT_NAME to be used in both build and release process
Reordered some of the variables in the sh files so that if there is an error in the build/release process the Default values are actually presented to the user (currently they show up as blank since the variables are not initilaized before the USAGE variable)
NOTE: This changes the service account name in the helm process as well. It changes it from ack-$SERVICE-controller to ack-$SERVICE-service-account. I am not sure if this is backwards compatible for when a controller is installed/updated via a helm deployment.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Signed-off-by: Adam D. Cornett adc@redhat.com

ack-bot · 2022-01-19T22:21:39Z

Hi @acornett21. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

RedbackThomson · 2022-01-20T19:49:44Z

/ok-to-test

RedbackThomson

Nice catch. Please see my inline comment for default service account name variables.

RedbackThomson · 2022-01-20T19:52:21Z

templates/config/controller/deployment.yaml.tpl

            drop:
              - ALL
      terminationGracePeriodSeconds: 10
+      serviceAccountName: ack-{{ .ServicePackageName }}-service-account


The Helm templates use {{ .ServiceAccountName }} here - https://github.com/aws-controllers-k8s/code-generator/blob/main/templates/helm/values.yaml.tpl#L75 . This references an optional parameter in the generator, which defaults to "default" -

code-generator/cmd/ack-generate/command/release.go

Line 46 in 60b04fe

&optServiceAccountName, "service-account-name", "default", "The name of the ServiceAccount AND ClusterRole used for ACK service controller",

Ideally we would use that same variable here, and update the default value for that optional parameter to be ack-<service>-service-account.

@RedbackThomson Thanks for pointing this out, I wasn't sure how this was getting picked up in helm. I have refactored to pass the same var to all necessary processes/flows. Let me know what you think, I had to add this to the API's process too, since a variable there is shared with the controller process.

RedbackThomson

Great. This is a big upgrade for the Kustomization users. Thanks Adam!

It changes it from ack-$SERVICE-controller to ack-$SERVICE-service-account

I prefer ack-$SERVICE-controller, since it's redundant to add service-account to the name of a service account. Especially since we are using the same values for the ClusterRole name.

RedbackThomson · 2022-01-21T17:56:39Z

cmd/ack-generate/command/root.go

 		&optAWSSDKGoVersion, "aws-sdk-go-version", "", "Version of github.com/aws/aws-sdk-go used to generate apis and controllers files",
 	)
+	rootCmd.PersistentFlags().StringVar(
+		&optServiceAccountName, "service-account-name", optServiceAccountName, "The name of the ServiceAccount AND ClusterRole used for ACK service controller",


I would initialize the default service account name in this flag as "", and then have a common method in common.go (like getSDKVersion) that you use as the getter, which returns the proper default value of ack-$SERVICE-controller.

Hmmm I also don't really like that we are using this value for the ClusterRole name, as well. Perhaps I will open an issue and add another flag, and move that stuff into its own variable later. For this PR it's cool with me, though.

This comment is misleading, its not the name of the ClusterRole it's just referenced in the ClusterRole. I'll update comments accordingly.

RedbackThomson · 2022-01-21T17:58:31Z

pkg/generate/ack/apis.go

-	CRD    *ackmodel.CRD
+	SDKAPI             *ackmodel.SDKAPI
+	CRD                *ackmodel.CRD
+	ServiceAccountName string


I don't believe we are using the ServiceAccountName variable in any of the templates used by the API command. I think we are free to strip it from this var struct, and from the API method declaration.

In API's no, but this struct is used in controller.go here and here, this is why it was refactored. Should a new struct be introduced in controller.go?

I'll revert the API changes, even though this struct is shared between the controller and the api code, the controller code does not need ServiceAccountName

RedbackThomson · 2022-01-21T17:59:14Z

pkg/generate/ack/controller.go

 	for _, path := range controllerConfigTemplatePaths {
 		outPath := strings.TrimSuffix(path, ".tpl")
-		if err = ts.Add(outPath, path, metaVars); err != nil {
+		if err = ts.Add(outPath, path, configVars); err != nil {


Oh interesting, this almost looks like a bug you found? Nice

Maybe a bug, but I don't think anything outside of metaVars was used in the templates, but now we need more then just metaVars

RedbackThomson · 2022-01-21T18:03:40Z

templates/config/controller/deployment.yaml.tpl

            drop:
              - ALL
      terminationGracePeriodSeconds: 10
+      serviceAccountName: {{ .ServiceAccountName }}


++ Nice forethought

acornett21 · 2022-01-21T19:46:54Z

I prefer ack-$SERVICE-controller, since it's redundant to add service-account to the name of a service account. Especially since we are using the same values for the ClusterRole name.

I went with service-account since this is how operator-sdk works, even though I do agree it is redundant, it does provide clarity. I'll revert it back to controller though if that makes more sense in for these controllers.

…rate controller process as well as ack-generate release process. Also adding in a service-account.yaml.tpl to be referenced in deployment.yaml.tpl and cluster-role-binding.yaml.tpl Signed-off-by: Adam D. Cornett <adc@redhat.com>

Issue #, if available: - Relates: aws-controllers-k8s/community#777 Description of changes: This pull request is needed to get tests to pass for - aws-controllers-k8s/code-generator#270 It also removes `sha` information from k8's versions, since this `sha` version is not static. Meaning that k8s can and does update a sha for a given version to fix security updates and some of these `shas` can no longer be pulled locally. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. Signed-off-by: Adam D. Cornett <adc@redhat.com>

RedbackThomson · 2022-01-25T19:10:26Z

Let's try these again with the new tests
/test all

acornett21 · 2022-01-25T19:39:23Z

/retest

RedbackThomson

Great. Let's send it!
/lgtm

ack-bot · 2022-01-25T20:05:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: acornett21, RedbackThomson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RedbackThomson]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-bot requested review from RedbackThomson and a-hilaly January 19, 2022 22:21

ack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 19, 2022

ack-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 20, 2022

RedbackThomson suggested changes Jan 20, 2022

View reviewed changes

acornett21 force-pushed the add_service_account branch 2 times, most recently from 342c8f9 to 180195d Compare January 21, 2022 17:29

acornett21 changed the title ~~adding in service-account.yaml.tpl to get away from using the default service account~~ refactoring build so that ServiceAccountName is available in ack-generate controller process as well as ack-generate release process Jan 21, 2022

RedbackThomson suggested changes Jan 21, 2022

View reviewed changes

acornett21 force-pushed the add_service_account branch from 180195d to 27e6276 Compare January 21, 2022 20:59

acornett21 force-pushed the add_service_account branch from 27e6276 to 162e61b Compare January 25, 2022 15:51

acornett21 mentioned this pull request Jan 25, 2022

Create ack-system namespace before applying RBAC test resources aws-controllers-k8s/test-infra#177

Merged

RedbackThomson changed the title ~~refactoring build so that ServiceAccountName is available in ack-generate controller process as well as ack-generate release process~~ Add parameter for service account name to all ackgenerate methods Jan 25, 2022

RedbackThomson approved these changes Jan 25, 2022

View reviewed changes

ack-bot assigned RedbackThomson Jan 25, 2022

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2022

ack-bot added the approved label Jan 25, 2022

ack-bot merged commit 6da05cd into aws-controllers-k8s:main Jan 25, 2022

Add parameter for service account name to all ackgenerate methods #270

Add parameter for service account name to all ackgenerate methods #270

Uh oh!

Conversation

acornett21 commented Jan 19, 2022 • edited by RedbackThomson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-bot commented Jan 19, 2022

Uh oh!

RedbackThomson commented Jan 20, 2022

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acornett21 commented Jan 21, 2022

Uh oh!

RedbackThomson commented Jan 25, 2022

Uh oh!

acornett21 commented Jan 25, 2022

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

ack-bot commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

acornett21 commented Jan 19, 2022 •

edited by RedbackThomson

Loading